Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async tests #1835

Merged
merged 5 commits into from
Jul 16, 2024
Merged

Add async tests #1835

merged 5 commits into from
Jul 16, 2024

Conversation

salomvary
Copy link
Contributor

As discussed with @tim-schilling in #1828 a good start towards adding async support is adding tests involving async views and async ORM usage.

Unfortunately, none of the automated tests showcase the problem described in #1828 as there is no ASGI equivalent of LiveServerTestCase and the issue seems to be specific to running an actual ASGI server.

Therefore, despite all tests are green, async support is still not there. Running the example app with an ASGI server using ASYNC_SERVER=true python example/manage.py runserver and visiting http://127.0.0.1:8000/async/db-concurrent/ does exhibit the deadlock though.

See also #1819.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply on this. Thank you for creating this PR!

I wonder if we could test this functionality by starting up runserver with the asgi application, then using a separate selenium test against that rather than using the Django test runner.

example/README.rst Show resolved Hide resolved
example/settings.py Outdated Show resolved Hide resolved
example/settings.py Outdated Show resolved Hide resolved
@tim-schilling
Copy link
Contributor

Closing in favor of #1938. Thank you for pushing this forward @salomvary! Your work here was helpful to the GSoC project to push the toolbar forward with async compatibility.

@tim-schilling
Copy link
Contributor

Nope, I was wrong. This still has value.

@tim-schilling
Copy link
Contributor

Rebased on main

@tim-schilling
Copy link
Contributor

@salomvary could you confirm that the deadlock issue is not a problem in this version? @salty-ivy removed the sql panel from async installations which seems to have resolved that issue. Granted, now the issue is that the toolbar doesn't have a SQL panel for async apps, but at least it doesn't lock up.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @salomvary for this! I'd still like your confirmation the deadlock has been handled when/if you have time.

@tim-schilling tim-schilling merged commit a9a66a9 into jazzband:main Jul 16, 2024
25 checks passed
@salty-ivy
Copy link
Member

salty-ivy commented Jul 16, 2024

@salomvary could you confirm that the deadlock issue is not a problem in this version? @salty-ivy removed the sql panel from async installations which seems to have resolved that issue. Granted, now the issue is that the toolbar doesn't have a SQL panel for async apps, but at least it doesn't lock up.

I think it was more of a django ORM issue rather than the toolbar itself. @salomvary mentioned it here
https://forum.djangoproject.com/t/are-concurrent-database-queries-in-asgi-a-thing/24136/2

@tim-schilling
Copy link
Contributor

Weird, on my machine it was executing them concurrently (I think). Or at least they didn't result in a deadlock.

@salomvary
Copy link
Contributor Author

Thank you @salomvary for this! I'd still like your confirmation the deadlock has been handled when/if you have time.

Yep, the deadlock was very likely not cause by the Debug Toolbar, it is a Django issue. As far as I can remember I was later able to reproduce the deadlock with Debug Toolbar not even installed.

@salty-ivy salty-ivy mentioned this pull request Aug 25, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants